Change MerginProject constructor to force one instance per project#283
Change MerginProject constructor to force one instance per project#283dvdkon wants to merge 2 commits intoMerginMaps:masterfrom
Conversation
mergin/merginproject.py
Outdated
| # directory paths to instances. | ||
| # The dictionary is a WeakValueDictionary so we don't cause memory leaks | ||
| # (when the instance is GC'd, the entry is deleted). | ||
| project_cache: weakref.WeakValueDictionary[str, "MerginProject"] = {} |
There was a problem hiding this comment.
I'm not sure if I understand... the type annotation is WeakValueDictionary - but doesn't project_cache become a normal dict if set it to {} ?
There was a problem hiding this comment.
Thanks, that's just me being an idiot, sorry. Though I wonder why my IDE didn't complain...
I've force-pushed a fix, since it's just one line.
dvdkon
force-pushed
the
project-single-instance
branch
from
0596d56 to
9ab77de
Compare
MarcelGeo
left a comment
MarcelGeo
left a comment
There was a problem hiding this comment.
Hi @dvdkon , could you run tests with your version of code? You probably do not need to fork repo, instead of this -> use clone.
This is good idea to do it in this way. I'm wondering if this is not like 🩹 for buggy behavior in db-sync itself. In plugin are flying the MerginProject instances and no issues with that. Yeah, we could probably solve initialisation of Geodiff. @wonder-sk
mergin/merginproject.py
Outdated
| return instance | ||
|
|
||
| instance = super().__new__(cls) | ||
| cls.project_cache[directory] = instance |
There was a problem hiding this comment.
Do we need to pass project_cache to class directly?
There was a problem hiding this comment.
No, it's already there since it's defined as a class property. cls.project_cache here is equivalent to MerginProject.project_cache.
Sadly I don't have the right (non-trial) account to run tests on the dev instance.
We could probably work around the desync issues by just being careful in |
|
On hold (see MerginMaps/db-sync#170) |
3 participants
Currently there can be multiple instances of
MerginProjectin memory that represent a single project (directory). This is an issue, because doing an operation on one instance will desync metadata in the others, which caused syncing to fail indb-sync.This PR changes the MerginProject constructor to return the existing instance for a given directory, if one exists. It also bumps the library version, so I can make subsequent changes in
db-sync.